Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Improve error handling when retrieving all detectors #267

Merged

Conversation

ohltyler
Copy link
Contributor

@ohltyler ohltyler commented Jul 31, 2020

Issue #, if available:

Description of changes:

This PR improves error handling in a few different places regarding retrieving all detectors from ES. Specifically:

  1. Displays a toast notification on dashboard and detector list pages if there was errors retrieving detectors
  2. Fixes issue of infinite loading state on detector list page when errors retrieving detectors
  3. Change to use AD redux store to identify if there was errors
  4. Properly handles rejected Promises on server side to prevent the plugin from getting in a bad state (e.g., could not retrieve property state from undefined).

Confirmed basic functionality of plugin works, and all UT/IT pass.

Screenshots:

Screen Shot 2020-07-31 at 9 13 52 AM

Screen Shot 2020-07-31 at 9 13 29 AM

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ohltyler ohltyler added the enhancement Enhance current feature for better performance, user experience, etc label Jul 31, 2020
@ohltyler ohltyler linked an issue Jul 31, 2020 that may be closed by this pull request
console.log(
'Anomaly detector - Unable to retrieve detector state',
err
return Promise.reject(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer you log the err. Same for below changes.

Copy link
Contributor Author

@ohltyler ohltyler Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can add in more error logs. The way it is currently implemented, there will still be an error log when the thrown errors get caught in the outermost catch() block of the function.

Copy link
Contributor

@yizheliu-amazon yizheliu-amazon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool. thanks for improving this.

Copy link
Contributor

@ylwu-amzn ylwu-amzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the fix!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Enhance current feature for better performance, user experience, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle errors when retrieving all detectors
3 participants